-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DYN-6607 - Input/Output Node - part 1 #14987
Conversation
- starting with the model and the test suite
- now types are contained inside an enum - added the basic primitives to the test structure, including lists checks - reworked the node to start getting the customization going (and make sense of the whole thing)
- created hierarchical container capable of tracking type inheritance - added all geometry tests
- finished all primitive date type tests
This reverts commit 1621855.
- completed tests checking inf inheritance works on individual and on list level
UI Smoke TestsTest: success. 2 passed, 0 failed. |
- removed the Enum, replaced directly with Type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good! Not sure what you know about and is on your todo list and what is a useful suggestion. We also don't need to make everything just right in this PR.
src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DefineData.cs
Outdated
Show resolved
Hide resolved
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void IsSupportedGeometryDataType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder at the exhaustiveness of these tests. Do we really need to check that every type in the dropdown validates? Is it useful to keep the full list of supported types here and in the dropdown items definition? Why isn't it enough to just check that the characteristics of the system work - test inheritance, is list/is not list, a type we know is not in the dropdown list, heterogenous list values?
Caveat: I don't have a lot of experience with TDD. Just seems like a lot of effort here for not a lot of validation value. @mjkkirschner?
- removed dictionary in favor of list of datatypes - renamed methods to better suit the specificity of the node functionality they were serving
/// A static dictionary for all Dynamo supported data types | ||
/// </summary> | ||
/// <returns>The dictionary containing the supported data types</returns> | ||
public static List<DataNodeDynamoType> GetDataNodeDynamoTypeList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this public?
why is it a list instead of an ienumerable or readonly collection? Do you expect callers to add types to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method seems inefficient, whenever this method is called it re evaluates allocating this list and allocating all these types... seems this could be done at compile time using an initializer - though I am not sure.
[IsVisibleInDynamoLibrary(false)] | ||
public static bool IsSupportedDataNodeDynamoType([ArbitraryDimensionArrayImport] object inputValue, string typeString, bool isList) | ||
{ | ||
var type = GetDataNodeDynamoTypeList().FirstOrDefault(x => x.Type.ToString().Equals(typeString)).Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not great, every time the node's AST is constructed we revaluate and reallocate the list of types, then do string comparisons? I think doing type comparisons will be much faster.
This reverts commit c90f105.
Purpose
This is the initial work regarding a new Input/Output node in Dynamo. The main function of the node is to enable custom type allocation (for all Dynamo-supported data types) and plays an important role in the larger ecosystem of infrastructural nodes inside Dynamo. On the level of the active graph, the node is a 'pass-through' node, meaning that the output is the same as the input.
The node has 4 main 'inputs', although only one that allows passing data form downstream:
Current state of affairs - node output is the validation result
This PR contains the main utility function that is used to evaluate the input. It also contains the starting bits of the node View customization (can be split if it's confusing).
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Reviewers
@saintentropy
@twastvedt
FYIs
@mjkkirschner